Conversation
Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4051 +/- ##
=======================================
Coverage 94.08% 94.08%
=======================================
Files 207 207
Lines 19217 19220 +3
=======================================
+ Hits 18081 18084 +3
Misses 938 938
Partials 198 198 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
The change looks good and the logic makes sense. Is there an upstream issue tracking this behaviour?
No. Must I make one? 😂 |
|
@gmlewis > Way to make AI do something useful for once! Good catch. Already merged so redundant for me to say this but LGTM! :) |
| requestBody := reader | ||
| if reader != nil { | ||
| // Wrap the provided reader so transport code does not observe concrete body types | ||
| // (for example *os.File) and switch to platform-specific sendfile fast paths. | ||
| // | ||
| // Why this exists: | ||
| // race-enabled test runs on Windows have surfaced data races in the sendfile path | ||
| // while request read/write loops run concurrently. Hiding concrete type information | ||
| // keeps uploads on the generic io.Reader copy path, which is race-stable and preserves | ||
| // request semantics (same bytes, same headers, same content length). | ||
| requestBody = uploadRequestBodyReader{Reader: reader} | ||
| } |
There was a problem hiding this comment.
This wrapper forces uploads onto the generic copy path in all builds and on all platforms. Could we limit this only to Windows, where the issue exists?
For Windows: wrap into uploadRequestBodyReader, for non-Windows use the original reader.
There was a problem hiding this comment.
good catch on the sendfile drop. tbh I'm leaning against splitting this into OS-specific build tags though.
since this is an API wrapper and not a CDN, the perf delta between kernel sendfile and a user-space copy for a github release asset is basically zero. maintaining github_windows.go and github_unix.go just to save a microsecond of cpu isn't really worth the friction.
also, the root cause is mostly just the Go -race detector freaking out on windows sockets, not a production bug. sticking to the generic copy path keeps the architecture clean across the board.
what do you think?
There was a problem hiding this comment.
Could we limit this only to Windows, where the issue exists?
I was taking the "safe route" thinking that if the data race could happen on Windows using the battle-tested Go standard library, then what is to say that it could not happen on other platforms?
Additionally, we have seen some "flaky tests" in this repo over the last year and I was hoping that this fix might eliminate them entirely.
I'm open to experimenting by changing this to a Windows-only fix if someone wants to make a PR, with the understanding (as always, actually) that we may need to revert it.
4 participants
This is an AI-assisted fix for the data race detected during the GitHub Actions workflows in #4050.
Since this is not an easy data race to understand (at least for me), I asked it to add comments as to why the changes being made are necessary to prevent the data race both in the CI/CD pipeline and potentially when using the library in production.
cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra - @munlicode
The original data race is below for posterity:
Details
From #4050 CI/CD: